[SPARK-56115] Support crds.install in Helm Chart#641
Conversation
TQJADE
changed the title
dongjoon-hyun
changed the title
crds.install in Helm Chart
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
I'm not sure if this PR is safe because templates/crds seems to be not protected during uninstallation. Could you double-check the CRD remains in the system during helm uninstall operation like before?
Does this PR introduce any user-facing change?
No
| doLast { | ||
| def generatedCRDFileBase = "$currentPath/build/resources/main/" | ||
| def stagedCRDFileBase = "$currentPath/../build-tools/helm/spark-kubernetes-operator/crds/" | ||
| def stagedCRDFileBase = "$currentPath/../build-tools/helm/spark-kubernetes-operator/templates/crds/" |
|
Why do we need to change |
Thanks for catching this! The move from crds/ to templates/crds/ did lose Helm's built-in uninstall protection. I've addressed this by adding a configurable crds.annotations value, defaulting to "helm.sh/resource-policy": keep, which tells Helm to skip deleting CRDs on uninstall. |
|
|
Could you check the CI failures because it fails 3 times consistently in our CI? Does it pass locally? 
|
Fix by commit: 4bfbb74 |
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
I'm wondering if you verify helm upgrade manually, too?
Thanks for pointing this out. Yes, upgrade will failed without labeling and annotations. Adding doc: 3825979#diff-ad74991b96593fdee570962fc5b1e319080abd55e16614f556f74b7de8fd02d1R161 |
There was a problem hiding this comment.
Hi, @TQJADE . Thank you for all your efforts and passion.
Given the current context, the potential side effects of this PR seem quite complex. Since we haven't had sufficient time to fully verify them, moving forward right now feels a bit risky. There's a concern that this might inadvertently lock us into supporting custom CRDs indefinitely, whereas the Apache Spark community is aiming for a more homogeneous infrastructure based on standard, community-blessed CRDs.
Therefore, I'd suggest we focus on releasing v1.0 without this feature for now, and then revisit this topic carefully for v2.0 once we've had time for more comprehensive testing.
2 participants
What changes were proposed in this pull request?
This PR introduces a crds.install configuration option to the Helm chart. CRD files are moved from the crds/ directory
(unconditionally installed by Helm) to templates/crds/ with a {{- if .Values.crds.install }} guard, allowing users to
disable CRD installation by setting crds.install: false. Similar to:
https://github.com/argoproj/argo-helm/blob/f7aee451eb788abd682d187fb055668bbfe28091/charts/argo-cd/values.yaml#L31-L40
Why are the changes needed?
In environments where CRDs are managed separately, users need the ability to skip CRD installation at the chart level. Previously the only option was the helm install --skip-crds flag, which is not declarative and cannot be expressed in a values file.
Does this PR introduce any user-facing change?
Yes.
releases.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.7